fix: emit layout change for batch node bounds #5939
Conversation
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 10/08/2025, 03:07:19 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/08/2025, 02:54:08 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
christian-byrne
force-pushed
the
vue-node/fix-layout-mutation
branch
from
5f682ef to
1a14c1c
Compare
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: fix: emit layout change for batch node bounds (#5939)
Impact: 113 additions, 16 deletions across 4 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 3
- Low: 3
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 2 issues
- Code Quality: 4 issues
Key Findings
Architecture & Design
The implementation follows good CRDT patterns by routing DOM-driven node bounds updates through a single operation type. The new BatchUpdateBoundsOperation properly integrates with the existing Yjs-based layout store architecture and maintains consistency with other operation handlers.
Security Considerations
No security issues identified. The implementation properly validates node existence and doesn't expose any attack vectors.
Performance Impact
Two performance considerations:
- Spatial index updates in loops may cause performance degradation for large batch operations
- Storing previousBounds increases memory footprint during batch operations
Integration Points
The integration with useLinkLayoutSync correctly handles link recomputation for updated nodes. The test coverage adequately validates the new functionality.
Positive Observations
- Good separation of concerns with dedicated operation type
- Proper integration with existing CRDT pipeline
- Comprehensive test coverage for the new functionality
- Consistent error handling patterns with existing codebase
- Clean type definitions for the new operation
Next Steps
- Address medium priority quality issues (null checks, type safety)
- Consider performance optimizations for large batch operations
- Replace arbitrary timeout in test with proper synchronization
- Validate OperationMeta interface usage in type definitions
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
christian-byrne
force-pushed
the
vue-node/fix-layout-mutation
branch
from
0f18640 to
5650ccb
Compare
dosubot
bot
added
the
size:L
Summary
Fixes issue where node size changes are not serialized by routing DOM-driven node bounds updates through a single CRDT operation so Vue node geometry stays synchronized with LiteGraph.
Changes
BatchUpdateBoundsOperationto the layout store, applied it via the existing Yjs pipeline, notified link sync to recompute touched nodes, and covered the path with a regression testReview Focus
Correctness of the new batch operation when multiple nodes update simultaneously, especially remote replay/undo scenarios and link geometry recomputation.
┆Issue is synchronized with this Notion page by Unito